[Component] SelectBox #4
Conversation
…rch, multiselect(show all options/ x more)
| label: string; | ||
| value: string; | ||
| type?: "standard"; | ||
| status?: "passed" | "failed" | "pending"; |
There was a problem hiding this comment.
Let's change status to match the status options for a property "valid" | "falsified" | "undetermined"
| onChange={(val) => setSortValue(val as string)} | ||
| /> | ||
| <SelectBox | ||
| options={selectBoxGroupedOptions} |
There was a problem hiding this comment.
On my machine I am seeing a ts error on this prop. Is that showing up on your end?
SelectBox.tsx(24, 5): The expected type comes from property 'options' which is declared here on type 'IntrinsicAttributes & SelectProps'
| onChange: (values: string[]) => void; | ||
| } | ||
|
|
||
| const Select: React.FC<SelectProps> = ({ |
There was a problem hiding this comment.
Using React.FC is a historically controversial way to define a component in React as such many project have moved away from doing this. As of TypeScript 5.1 and React 18, React.FC is now officially 'fine' but many project still do not use this. This way of defining a function also doesn't match the other ways functions were defined and it is important to keep code consistent across the codebase at times where it makes sense to do so. Is there a particular reason you chose to use React.FC here?
| }) => { | ||
| const [isOpen, setIsOpen] = useState<boolean>(false); | ||
| const [searchValue, setSearchValue] = useState<string>(""); | ||
| const [selectedValues, setSelectedValues] = useState<string[]>([]); // values of slected options |
There was a problem hiding this comment.
The state for the selectedValues is state that will impact more than just this component so it should be lifted up out of this component and passed through as a prop just like what was done in the dropdown component.
| const chipsContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const theme = { | ||
| textOnSurface: "text-gray-900 dark:text-gray-100", |
There was a problem hiding this comment.
onSurface is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.
|
|
||
| const theme = { | ||
| textOnSurface: "text-gray-900 dark:text-gray-100", | ||
| bgContainer: "bg-white dark:bg-gray-700", |
There was a problem hiding this comment.
container is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.
| const theme = { | ||
| textOnSurface: "text-gray-900 dark:text-gray-100", | ||
| bgContainer: "bg-white dark:bg-gray-700", | ||
| bgContainerHigh: "bg-gray-100 dark:bg-gray-800", |
There was a problem hiding this comment.
containerHigh is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.
| textOnSurface: "text-gray-900 dark:text-gray-100", | ||
| bgContainer: "bg-white dark:bg-gray-700", | ||
| bgContainerHigh: "bg-gray-100 dark:bg-gray-800", | ||
| borderOutline: "border-gray-300 dark:border-gray-600", |
There was a problem hiding this comment.
outline is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.
| const inputRef = useRef<HTMLInputElement>(null); | ||
| const chipsContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const theme = { |
There was a problem hiding this comment.
Please remove this theme object and define all the styles inline, all but one of the style options in this theme options have already been defined in the global stylesheet or are only applied in one location.
| }; | ||
|
|
||
| return ( | ||
| <div ref={componentRef} className="relative w-full font-inter"> |
There was a problem hiding this comment.
Why is the font set to inter here?
| } | ||
| }; | ||
| document.addEventListener("mousedown", handleClickOutside); | ||
| return () => document.removeEventListener("mousedown", handleClickOutside); |
There was a problem hiding this comment.
If you used a 'click' event listener in dropdown why did you choose to use a 'mousedown' click event in this listener?
| showAllSelected = false, | ||
| onChange = () => {}, | ||
| }) => { | ||
| const [isOpen, setIsOpen] = useState<boolean>(false); |
There was a problem hiding this comment.
Can you name these state variables to [open, setOpen] that way we are ensuring consistent naming in all the components? The dropdown component has the naming for this similar state that I am requesting.
| e.stopPropagation(); | ||
| if (!isOpen) setIsOpen(true); | ||
| }} | ||
| placeholder={placeholderVisible ? placeholder : ""} |
There was a problem hiding this comment.
Can you explain to me what this conditional statement is for?
| type="text" | ||
| value={inputDisplayValue} | ||
| onChange={(e) => setSearchValue(e.target.value)} | ||
| onClick={(e) => { |
There was a problem hiding this comment.
If the parent element already handles changing the open state, why is this onClick event needed?
| if (!isOpen) setIsOpen(true); | ||
| }} | ||
| placeholder={placeholderVisible ? placeholder : ""} | ||
| readOnly={!search || (!isOpen && !!selectedItem)} |
There was a problem hiding this comment.
Can you please explain what functionality you are trying to achieve with this conditional statement?
| options={selectBoxGroupedOptions} | ||
| placeholder="Select Properties" | ||
| search={true} | ||
| multiSelect={false} |
There was a problem hiding this comment.
The default value for this prop is false so you don't need to redefine a value of false here as well.
| <SelectBox | ||
| placeholder="Select Properties" | ||
| options={flatSelectBoxOptions} | ||
| multiSelect={false} |
There was a problem hiding this comment.
The default value for this prop is false so you don't need to redefine a value of false here as well.
| isOpen ? "rotate-180" : "rotate-0" | ||
| } ${theme.textOnSurface}`} | ||
| > | ||
| <ChevronDown /> |
There was a problem hiding this comment.
This icon isn't showing up in the UI for me.
|
|
||
| // Focus input when opening and searchable | ||
| if (!isOpen && search && inputRef.current) { | ||
| setTimeout(() => inputRef.current?.focus(), 0); |
There was a problem hiding this comment.
Can you please explain what you are attempting to do with this conditional setTimeout statement?
|
Reworking on the PR to be in sync with changes of Dropdown component as in 8230865; updating and reusing Menu |
Pull request template
Description
Details of this SelectBox component and its behaviour is as per this document
Task:
Checklist